refactor(core): move the command descriptor registry into its own workspace package - #2348
refactor(core): move the command descriptor registry into its own workspace package#2348thymikee wants to merge 5 commits into
Conversation
…kage The descriptor registry, its catalogs, and the two parsing leaves it owns (`wait-positionals`, `parse-timeout`) depended on nothing in root `src/`: their only imports are `@agent-device/contracts` and `@agent-device/selectors`. They were nonetheless the daemon's largest root coupling, and under R11 the reason the daemon client and the session event journal cannot become packages. They move as-is into `@agent-device/command-registry`, one exported subpath per module pointing directly at the moved source file, and every consumer switches to the owning specifier. No compatibility re-export stays behind at the old path. The `__OWNER_FILES__` build-flag declaration moves with its only consumer; the root program and the SDK examples include it from its new home. Six descriptor tests reach into root `src/` (daemon parity, ref-frame classification, the CLI-route coherence gate, the cross-surface post-action-observation gate, the timeout-policy gate, and the shutdown mechanics gate), which a package may not do, so they stay in root beside the other cross-surface suites; the rest move unchanged with their subjects. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016DkNZCMWtm1KUbseKANADi
The rules that name the registry by path follow it: R66's `COMMAND_DESCRIPTOR_MODULE`, R16's record-runtime join subject, and the Fallow `AssertTrue` totality-guard key. The two descriptor hubs leave `HUB_ENTRY_FILES` because the package manifest now publishes them — the eager-closure gate discovers them as facades, and one entry gets one rule. `command-registry` joins the ranked spine at rank 1, beside the contracts and selectors it depends on. Two illustrative paths in gate comments and a model test are re-pointed at root files that still exist. No `APPROVED_OVER_CEILING` row: rename detection carries every moved entry's merge-base baseline, so all twelve fall under the no-growth rule rather than a ceiling. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016DkNZCMWtm1KUbseKANADi
|
Size Report
Startup median (7 runs, lower is better):
|
|
Android / Smoke Tests failed on The failing step is the live emulator orientation check in
This lane is also independently red on recent I have re-run the failed job once (my one allowed re-run). Every other completed check on this head is green: Lint & Format, Typecheck & Package, Integration Tests, Compatibility & Provenance, Bundle Size, Ownership and context budgets, command-docs-gate, CodeQL, and the other Smoke Tests lane. If the re-run fails the same way, that is evidence about the emulator lane, not this diff, and I will say so rather than change the PR to chase it. Generated by Claude Code |
|
The package extraction and gates are otherwise clean at e18aa8f, and CI is green. One source-checkout regression blocks readiness: the daemon signature walker follows only relative imports. The registry and derive modules were previously in that graph; their new workspace imports are ignored. Editing the moved command policy can therefore reuse a daemon running the old implementation. This shares the signature-resolution fix needed by #2347. Please make that a shared prerequisite rather than adding a second resolver, and prove a package-owned registry edit changes the source daemon signature. Bundled installations are not affected. |
thymikee
left a comment
There was a problem hiding this comment.
Adversarial review from the #2335 coordinator, at e18aa8f. I verified the claims against the tree rather than reading the body — the substantive ones hold up. Three things to record before this merges; none block.
Verified independently: old paths are gone with no re-export shim (src/command-catalog.ts and src/core/command-descriptor/ absent from the tree); zero suppressions added anywhere in the diff (no fallow-ignore, APPROVED_OVER_CEILING, ts-expect-error, disable comments, skipped tests); enforcement edits are confined to e18aa8f and none leaked into the implementation commit; all 12 subpaths point directly at moved source files with no index.ts.
1. The HUB_ENTRY_FILES deletion deserves an explicit note — it is not a pure relocation.
Dropping registry.ts and platform-execution-entry.ts from that list is the one edit here whose failure mode is silent, so I checked it rather than taking the commit message's word: discoverFacadeEntryFiles delegates to R11's manifest-first facadeEntryFiles, and the new manifest publishes both ./registry and ./platform-execution-entry straight at the moved sources, so both are rediscovered. That part is correct.
What the commit message does not say is that the polarity of denyPlatformImplementations flips with the move: hub rows set it false, package entry surfaces set it true. These two files are now held to the stricter rule. That is fine — and green — but a future reader comparing the lists will read a deletion as lost coverage. One sentence in the commit body stating the flip, and that it was intentional, converts a silent semantic change into a recorded one.
2. Baseline discrepancy against the issue — worth settling now, not at the end.
#2336 states 154 daemon → root src/ value edges; you measured 150 → 114 with scripts/depgraph/build.ts. The delta is −36, exactly the issue's predicted reduction, so the change is right and the ceiling is met either way. But the umbrella (#2335) promises a final re-measurement of this number, and it was originally taken over a "production-only import graph (1,629 files, 8,805 edges)". Please name the filter difference (production-only vs. what you counted) in the PR body so the closing measurement uses one definition rather than silently comparing two.
3. Name the six stranded tests' root dependency.
src/__tests__/command-descriptor-*.test.ts stay behind because a package may not import root src/. That is the correct call, but it leaves AGENTS.md's "tests mirror source topology one-to-one" broken for six files, and the reason is now invisible. List which root-src/ symbol each of the six actually needs. That turns an acknowledged exception into an inventory a later wave can act on — and it is the same kind of residual-seam list #2342 is required to produce.
For the record, the three chmod 0500 unit failures are not yours: the container runs as uid 0, which ignores the mode. #2347 hit the identical three independently, and CI is green here across all 21 checks.
Generated by Claude Code
The source-checkout code signature walked relative specifiers only, so every module a workspace package owns was outside it: the client compared a daemon against a fingerprint that could not see the package half of the graph, and editing a package-owned module left the signature unchanged, so `isReusableDaemonInfo` kept reusing a daemon running the superseded code. This PR moves the command descriptor registry across that boundary, which is what surfaced it; kernel, contracts, capture-kit and every other package have been invisible since #1490. The walk now follows `@agent-device/*` specifiers too, rooted at the checkout it was handed rather than at this module's own location, so two sibling checkouts cannot fingerprint each other's packages. Resolution goes through `<root>/node_modules`, the link the loader itself follows, so the package directory comes from the install rather than from a guess that a package's directory is named after it; the manifest that chose the module joins the graph beside it, because its `exports` map IS the resolution and the cache replays a walk's whole input from `statSync`. Manifests are stamped but not scanned: JSON names no imports, and the graph stays the size of what the daemon actually loads. Bundled installs are unaffected: the dist entry carries no workspace specifiers (tsdown bundles them), so that arm walks exactly as before. Measured on this checkout: 608 modules to 1,460, the cold walk 35ms to ~80ms, and the cached per-invocation path 3.1ms to 7.5ms. The two doc comments quoting the old graph size are updated. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016DkNZCMWtm1KUbseKANADi
Following both specifier kinds inline pushed walkDaemonCodeGraph past the complexity threshold. The per-file question — which edges does this file contribute — is its own function, so the walk reads as visit, stamp, enqueue again, and the rule that a manifest is stamped but never scanned for specifiers lives with the resolution it qualifies. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016DkNZCMWtm1KUbseKANADi
|
This head now conflicts with main. Resolve that alongside the shared source-signature prerequisite from #2347, then rerun the affected gates. |
|
Fixed in One resolver, in the file #2347 moves. The fix is inside Three properties worth review:
Proof. Cost, measured here: 608 → 1,460 modules; cold walk 35ms → ~80ms; the per-invocation cached path 3.1ms → 7.5ms. The two doc comments quoting "~800 modules / ~30ms" are updated rather than left stale.
Generated by Claude Code |
Takes #2347's move of the daemon code-signature walker into `@agent-device/host-kit` together with its workspace-specifier resolution, and drops this branch's own copy of that fix: one resolver, the one on main. The claim this branch owed keeps its own test beside the walker — the daemon source graph stamps `packages/command-registry/src/registry.ts`, `catalog.ts` and the manifest, measured at 1,461 modules, and touching the registry changes the signature. `src/cli/commands/router-types.ts`: #2349's `CommandProgressState` import alongside this branch's catalog specifier. A fixture string in the relocated CLI-route test also goes back to the specifier it illustrates; the move rewrote it as if it were a real import. Three review points, recorded here because their commits are pushed: - Dropping `registry.ts` and `platform-execution-entry.ts` from `HUB_ENTRY_FILES` is not only bookkeeping: the package manifest now publishes them, so the gate discovers them as domain facades and `denyPlatformImplementations` flips false to true. Both now carry the ADR-0019 assertion that they evaluate no concrete platform implementation, and both pass it. That strengthening is intended. - The 150 this branch reports against the issue's 154 is a filter difference, not a discrepancy: `pnpm depgraph` collapses to one edge per file pair, and 150 counts value edges only. Counting type-only and dynamic edges on the same graph gives 174 into 67 root files, which is the shape the issue's own per-file breakdown uses; both readings drop by exactly the 36 the issue predicts. - The six tests that stay in root do so for one root symbol each: parity needs STRUCTURED_BATCH_COMMAND_NAMES, DAEMON_COMMAND_DESCRIPTORS, canRunReplayScopedAction and DaemonRequest; ref-frame-effect needs DAEMON_COMMAND_DESCRIPTORS, resolveRefFrameEffect and DaemonRequest; timeout-policy needs DEFAULT_STABLE_TIMEOUT_MS; platform-execution-cli-route needs CLI_INJECTED_DAEMON_DISPATCHES and CliInjectedRoute; post-action-observation needs AgentDeviceClient, getCliCommandSchema, readInputFromCli, findCommandMetadata, the two command-family registry listers, SettleCapableClientOptionCommands, buildActionDetails and COMMAND_OUTPUT_SCHEMAS; shutdown-runtime-execution imports nothing but reads the daemon and root host sources whose text it pins. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016DkNZCMWtm1KUbseKANADi
|
Merged Signature coverage, verified after the merge. #2347 landed the same fix in Conflict: only The three review points are recorded in the merge commit body. In short: dropping the two entries from Re-measured on the new base: daemon → root Generated by Claude Code |
|
Signature-walk cost, re-measured on this head against pre-#2347
The last row is the one on the hot path: Of this package: 11 imported sources plus Two doc comments in Generated by Claude Code |
Summary
src/core/command-descriptor/,src/command-catalog.ts,src/core/wait-positionals.tsandsrc/core/parse-timeout.tsimported nothing from rootsrc/, yet were the daemon's largest root coupling. They move as git renames into a new private package@agent-device/command-registry(deps: contracts, selectors), one subpath per module pointing straight at the moved file; noindex.ts, no re-export at the old path. Every consumer — daemon, commands, mcp, cli, cli-schema, core, root, client, ai-sdk, scripts, tests — switches to the owning specifier; contracts is untouched. Six descriptor tests that import rootsrc/stay there, which a package may not do; the rest move unchanged. The__OWNER_FILES__declaration moves with its only consumer.Merged
mainatb4ebd77, taking #2347's code-signature walker and dropping this branch's duplicate of the same fix. What survives is a test pinning that the daemon graph reaches this package, and two doc comments corrected off their pre-#2347 figures.Rename-only proof —
git diff -M90% --stat origin/main...HEAD: 42 renames, all 12 moved sources at 92–100%.Closes #2336.
Validation
pnpm check:affected --runatfc40090: 11 checks green; vitest-related has 9744 passed, 3 failed — thechmod 0500tests that fail identically atorigin/mainhere, since uid 0 ignores the mode.Daemon → root
src/value edges 148 → 112 (−36, ceiling 118), frompnpm depgraph. CLI help byte-identical tob4ebd77: 82 sections, one sha256.Signature walk, measured against pre-#2347
main: 620 → 1,461 modules, 12 of them this package; cold walk ~90ms; and the CLI hot path — the cached per-invocation signature — 2.9ms → 7.5ms.🤖 Generated with Claude Code
https://claude.ai/code/session_016DkNZCMWtm1KUbseKANADi